-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: custom cargo config to reduce build time #2104
chore: custom cargo config to reduce build time #2104
Conversation
bb960f5
to
128d3f3
Compare
128d3f3
to
4a2c4c4
Compare
3d3548d
to
23051c0
Compare
2c6d309
to
89c9597
Compare
999a06c
to
6cc0ba9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks promising!
- name: Install MDBook | ||
run: cargo install mdbook mdbook-linkcheck | ||
continue-on-error: true | ||
- name: Setup sccache | ||
uses: hanabi1224/sccache-action@v1.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be great to include a comment on why are we using a fork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Basically we need the cache-update
to only save the cache on 1. main
branch, 2. cache not found. The original action repo looks like unmaintained so we keep using the fork
run: make udeps | ||
- run: make test-all | ||
env: | ||
CC: "sccache clang" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it wouldn't clash with some other steps, but would adding those envs as global for the entire workflow be fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sccache
is not always the first step to avoid generating cache for cargo install [tools]
, so that sccache
is not available during cargo install
and apt-get install
which very likely invoke c/c++
compilers
0b0646d
to
5dbcbd7
Compare
Hey @lemmih I think it requires manual changes to the branch rule of required CI checks before this PR can be merged. Could u help with that? |
Done. |
Summary of changes
Changes introduced in this pull request:
.cargo/config.toml
to improve build performance, for details checkout https://jondot.medium.com/8-steps-for-troubleshooting-your-rust-build-times-2ffc965fd13erust-cache
withsccache-action
, also change the cache strategy to write on push tomain
branch only.cargo install
ed tools (e.g. spellcheck from ~200MB to ~30MB)rust-cache saves >8GB cache on every push to
main
and PR branch, while we only have 10GB total cache size, which makes cache unreliable when github automatically purges themWith the new strategy, PR commit will read the cache generated from the latest
main
commit but not saving them. (github caches are immutable, so saving means duplicating by appending some suffix to the cache key)The cache size each
main
commit saves is now reduced to ~1-2GB.Regarding build time, here is an example run with the new strategy, looks on par or slightly faster than before
The slowest udeps job takes ~15min in previous run, and all-in-one lint job takes ~9min with the new strategy. (They might run on different CPU tho, the hosted runner has a pool of CPU models)
Reference issue to close (if applicable)
Closes
Other information and links